Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace more .all imports #35372

Merged
merged 8 commits into from
Apr 6, 2023
Merged

Replace more .all imports #35372

merged 8 commits into from
Apr 6, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 28, 2023 •
edited
Loading

📚 Description

This is a follow-up on:

As preparation for #35322, which is changing more packages to implicit namespace packages, we remove .all imports from these packages throughout the Sage library.

This is part of:

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@remyoudompheng
Lighter construction of finite field elements from lists
63c3ab6 
When doing intensive polynomial arithmetic with the NTL implementation
the constructor with lists is called a large number of times
and may spend a lot of time constructing the vector_space and
FreeModuleElement objects.

The very common call to vector_space(map=False) is optimized to be as cheap as
possible using the already cached object.

The common case of lists of length 0 and 1 is replaced by cheaper
shortcuts.
@mkoeppe mkoeppe self-assigned this Mar 28, 2023
@tornaria
Copy link
Contributor

tornaria commented Mar 31, 2023 •
edited
Loading

EDIT: all my problems were fixed by touch src/sage/__init__.py which has the immediate effect of isolating the current sage package from the system (as in: the presence of this __init__.py file "closes" the sage package and nothing else will be taken from the system site-packages).

I'm leaving the original comment below in case someone has a similar issue. The problem below was caused by sage/modules/__init__.py existing in the system site-packages, but not existing in the local tree, so the sage.modules package (and all its modules) get imported from site-packages instead of locally. And of course sage.modules.with_basis.all doesn't exist in system site-packages (but it is expected to exist in the local tree)...

You are just making my life miserable:

ModuleNotFoundError: No module named 'sage.modules.with_basis.all'

I did nuke all the empty __init__.py files in my system site-packages. Then my system sagemath is broken because I'm missing some all.py files. Also, some non-empty __init__.py files have to be removed as here. Producing a patch from #35322, #35366 and this to apply to 9.8 is a pain because of the huge amount of churn. Also, patching 9.8 is a moving pain since what has to be done changes depending on which of these three I want to test...

One thing I don't understand is: why on earth are the empty all.py files needed? Aren't you repeating python mistakes? It feels crazy that some doctest failures disappear just by adding an empty __init__.py or an empty all.py.

I would like to argue that the behaviour with or without these empty files should be identical. That's the point of PEP 420: that directories without __init__.py are packages just as much as those with __init__.py files. Requiring an all.py file goes against this.

In any case, as I requested before, it would be better if there is JUST ONE single point of "removing empty files that may break havoc" and then don't do it again.

Even more: could we please have

(1) one PR that introduces all the changes needed to support removing empty __init__.py files, but leaves all the empty __init__.py files there? The goal is that I get something where I can safely do

find  .../sage/ -name "__init__.py" -size 0 -delete

without having any type of trouble and without having to add random all.py files.

(2) a separate PR with only removal of empty __init__.py files ?

Ideally (1) is merged in one release, and (2) is merged in a later release.

After the move to gh, I was able to contribute a lot more efficiently to sage (both code and reviews). But the last 2 weeks, since this happened with the merge of #35100, I've been stuck unable to do anything at all. I'm getting tempted of pruning all my sage git trees for 6 months to wait until this is over.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 31, 2023

Kindly rephrase for clarity

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 1, 2023

Rebased away from #35322. Ready for review.

@mkoeppe mkoeppe requested a review from fchapoton April 2, 2023 07:53
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 2, 2023

Rebased on top of #35418

@tornaria
Copy link
Contributor

tornaria commented Apr 4, 2023

Merge conflict with #35358 (see 63c3ab6)

@tornaria
Copy link
Contributor

tornaria commented Apr 4, 2023

FWIW, after fixing the trivial merge conflict, all tests pass for me on void linux.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2023

Merged & resolved

@tornaria
Copy link
Contributor

tornaria commented Apr 4, 2023

LGTM

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2023

Thanks!

@vbraun vbraun merged commit 519b62c into sagemath:develop Apr 6, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tornaria tornaria tornaria approved these changes

@fchapoton fchapoton Awaiting requested review from fchapoton

Assignees

@mkoeppe mkoeppe

Labels
t: refactoring
Projects
None yet
Milestone
sage-10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mkoeppe @tornaria @vbraun @remyoudompheng @fchapoton